feat(analysis/offline):Added a filter for good events#1280
feat(analysis/offline):Added a filter for good events#1280pgrusell wants to merge 4 commits intoR3BRootGroup:devfrom
Conversation
pgrusell
force-pushed
the
tofdhit
branch
3 times, most recently
from
b042b04 to
c7ffddf
Compare
pgrusell
force-pushed
the
tofdhit
branch
2 times, most recently
from
388767d to
c7ffddf
Compare
refactor(r3bdata/tofdHitData): TofdHitData no longer inherits R3BHit, as it's not used by any other detector data. Moreover, method to get the plane id has been change from DetId to PlaneId feat(analysis/offline): Class to filter events according to AoQ at FRS and Q at ToFD feat(analysis/online): added method to select the charge selection algorithm style(analysis/online): Clang format applied fix(analysis): clang-tidy feat(analysis/online): change double array to std::vector style(analysis/online): clang-format
There was a problem hiding this comment.
Pull request overview
This PR refactors the R3BTofdHitData class to no longer inherit from R3BHit and renames the method GetDetId() to GetPlaneId() for better semantic clarity. It also introduces a new event filter class R3BEventFilter for offline analysis that filters events based on FRS A/Q and ToFD charge selections.
Changes:
- Refactored
R3BTofdHitDatato inherit directly fromTObjectinstead ofR3BHit, with inline getters and modern C++ features - Renamed
GetDetId()toGetPlaneId()across all usages in tracking and online analysis files - Added new
R3BEventFilterclass for filtering events based on FRS and ToFD criteria - Updated header guards to use
#pragma oncein several files - Code formatting improvements in
R3BFiberTrackingOnlineSpectra.cxx
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| r3bdata/tofData/R3BTofdHitData.h | Complete refactoring: removed R3BHit inheritance, added inline getters, changed GetDetId to GetPlaneId, added deprecated methods for backward compatibility |
| r3bdata/tofData/R3BTofdHitData.cxx | Updated constructor to initialize member variables directly, removed old method implementations |
| tracking/R3BTrackingS515.cxx | Updated method call from GetDetId() to GetPlaneId() |
| tracking/R3BTrackingG249.cxx | Updated method call from GetDetId() to GetPlaneId() |
| tofd/online/R3BTofDOnlineSpectra.cxx | Updated method call from GetDetId() to GetPlaneId() |
| analysis/online/R3BTofDvsTttxOnlineSpectra.cxx | Updated method call from GetDetId() to GetPlaneId() |
| analysis/online/R3BFibervsTofDOnlineSpectra.cxx | Updated method call from GetDetId() to GetPlaneId() |
| analysis/online/R3BFiberTrackingOnlineSpectra.cxx | Updated method call from GetDetId() to GetPlaneId() and code formatting improvements |
| analysis/online/R3BCalifavsTofDOnlineSpectra.cxx | Updated method call from GetDetId() to GetPlaneId() and converted C-style arrays to std::vector |
| analysis/offline/R3BEventFilter.h | New event filter class header |
| analysis/offline/R3BEventFilter.cxx | New event filter class implementation |
| r3bdata/frsData/R3BFrsData.h | Changed header guard to #pragma once |
| analysis/offline/R3BIncomingBeta.h | Changed header guard to #pragma once |
| analysis/AnaLinkDef.h | Added R3BEventFilter to ROOT dictionary |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Deprecated methods | ||
| [[deprecated("Use GetPlaneId() instead.")]] UInt_t GetDetId() const { return fPlaneId; } | ||
| [[deprecated]] UInt_t GetHitId() const { return -1; } |
There was a problem hiding this comment.
The deprecated GetHitId() method returns -1, but the return type is UInt_t (unsigned integer). This will cause -1 to be converted to a large unsigned value (typically 4294967295 on 32-bit systems). Consider either changing the return type to Int_t or returning 0 instead.
| [[deprecated]] UInt_t GetHitId() const { return -1; } | |
| [[deprecated]] UInt_t GetHitId() const { return 0; } |
| if (planes13ok || planes24ok) | ||
| return true; | ||
| else | ||
| return false; | ||
| } | ||
|
|
||
| inline bool CheckFireConditionsAllPlanes(const std::vector<int>& v) | ||
| { |
There was a problem hiding this comment.
Simplified return statement recommended. The if-else can be replaced with a direct return of the boolean expression.
| if (planes13ok || planes24ok) | |
| return true; | |
| else | |
| return false; | |
| } | |
| inline bool CheckFireConditionsAllPlanes(const std::vector<int>& v) | |
| { | |
| return planes13ok || planes24ok; | |
| } | |
| inline bool CheckFireConditionsAllPlanes(const std::vector<int>& v) | |
| { | |
| inline bool CheckFireConditionsAllPlanes(const std::vector<int>& v) | |
| { |
| void R3BEventFilter::Exec(Option_t*) | ||
| { | ||
| bool fValidEvent = false; | ||
|
|
There was a problem hiding this comment.
Potential null pointer dereference. The fHeader pointer is used without null checking on line 141. If fHeader is nullptr, this will cause a crash. Add a null check for fHeader before accessing GetExpId().
| if (!fHeader) | |
| { | |
| R3BLOG(error, "R3BEventFilter: Event header not available, skipping event"); | |
| StoreEvent(false); | |
| return; | |
| } |
| auto frsdata = dynamic_cast<R3BFrsData*>(fFrsData->At(ihit)); | ||
| auto charge = frsdata->GetZ(); | ||
| auto aoq = frsdata->GetAq(); |
There was a problem hiding this comment.
Missing null check after dynamic_cast. If the cast fails, frsdata will be nullptr and calling GetZ() or GetAq() on it will cause a crash. Add a null check after the dynamic_cast.
| auto tofdhit = dynamic_cast<R3BTofdHitData*>(fTofdHit->At(ihit)); | ||
| auto plane = tofdhit->GetPlaneId(); | ||
|
|
||
| // Add a method to calculate the charge in each plane | ||
| chargeSelected[plane - 1] = isSelectedCharge(plane, tofdhit->GetChargeZ()); |
There was a problem hiding this comment.
Missing null check after dynamic_cast. If the cast fails, tofdhit will be nullptr and calling GetPlaneId() or GetChargeZ() on it will cause a crash. Add a null check after the dynamic_cast.
| inline bool CheckFireConditionsTwoPlanes(const std::vector<int>& v) | ||
| { | ||
|
|
||
| bool planes13ok = false; | ||
| bool planes24ok = false; | ||
|
|
||
| if ((v[0] == 1) && (v[2] == 1)) | ||
| planes13ok = true; | ||
|
|
||
| if ((v[1] == 1) && (v[3] == 1)) | ||
| planes24ok = true; |
There was a problem hiding this comment.
Missing bounds check. The function accesses v[0], v[1], v[2], and v[3] without verifying that the vector has at least 4 elements. Add a size check similar to CheckFireConditionsAllPlanes to prevent out-of-bounds access.
| } | ||
| } | ||
|
|
||
| // Function to determinate the charge in terms of the limits |
There was a problem hiding this comment.
Typo in comment: "determinate" should be "determine".
| // Function to determinate the charge in terms of the limits | |
| // Function to determine the charge in terms of the limits |
| auto plane = tofdhit->GetPlaneId(); | ||
|
|
||
| // Add a method to calculate the charge in each plane | ||
| chargeSelected[plane - 1] = isSelectedCharge(plane, tofdhit->GetChargeZ()); |
There was a problem hiding this comment.
Potential out-of-bounds access. The plane ID from GetPlaneId() is used to index into chargeSelected and fChargeLimits without validation. If plane is 0 or greater than 4, this will cause out-of-bounds access. Consider adding bounds checking to ensure plane is within the valid range (1-4) before using it as an array index.
| auto isSelectedCharge = [&](int plane, double charge) | ||
| { | ||
| if ((charge > fChargeLimits[plane - 1][0]) && (charge < fChargeLimits[plane - 1][1])) | ||
| return 1; | ||
| else | ||
| return 0; |
There was a problem hiding this comment.
The lambda function isSelectedCharge accesses fChargeLimits[plane - 1] without validating that the index is within bounds. If plane is 0 or if fChargeLimits has fewer than 'plane' elements, this will cause undefined behavior. Add bounds checking before accessing the vector.
| } | ||
|
|
||
| Int_t nHits = fHitItemsCalifa->GetEntriesFast(); | ||
| UInt_t nHits = fHitItemsCalifa->GetEntriesFast(); |
There was a problem hiding this comment.
Type mismatch: nHits is declared as UInt_t but the loop counter ihit on line 166 is Int_t. This can cause a signed/unsigned comparison warning. Consider making ihit also UInt_t or keeping nHits as Int_t for consistency with GetEntriesFast() which returns Int_t.
| UInt_t nHits = fHitItemsCalifa->GetEntriesFast(); | |
| Int_t nHits = fHitItemsCalifa->GetEntriesFast(); |
refactor(r3bdata/tofdHitData): TofdHitData no longer inherits R3BHit, as it's not used by any other detector data. Moreover, method to get the plane id has been change from DetId to PlaneId
feat(analysis/offline): Class to filter events according to AoQ at FRS and Q at ToFD
feat(analysis/online): added method to select the charge selection algorithm
Checklist:
devbranch